Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[F3D] Don´t hide properties that will get exported (specifically geo modes) #379

Merged
merged 6 commits into from
Jul 24, 2024

Conversation

Lilaa3
Copy link
Collaborator

@Lilaa3 Lilaa3 commented Jul 12, 2024

There was two options in fixing this:
Not exporting hidden modes or always showing all modes
I went with the latter in this pr as its more logical, nonsense example but if a game always defaults to packed normals but doesn´t default to lighting, that should still be setable AND visible in the world settings. Or maybe one could use a hack that revolves around checking for an unused mode being on. In my opinion this is the best way to do it and it doesn´t add much bloat to the UI even in ex3.
I also fixed a bug where if texgen was on without lighting it would still emulate texgen, even though it was neither visible or correct

@sauraen
Copy link
Contributor

sauraen commented Jul 18, 2024

I don't agree with this PR. When I redesigned the geometry mode UI (which was needed for F3DEX3, but applies to all microcodes), I found it confusing to remember which features depended on which other features, even for features I made myself. Hiding features which you cannot have without other features, e.g. things dependent on lighting, is an easy way to represent this; the alternative would be to have a ton of warnings and no structure in the UI.

It is true that you can import and export a state which is not visible in the UI, e.g. texgen without lighting. However:

  1. You can actually edit this state, e.g. by enabling lighting, enabling/disabling texgen, then disabling lighting.
  2. While the UI does not correctly reflect the F3D display list contents imported/exported, it does correctly reflect the in-game rendering results on N64. For example, if the display list says texgen without lighting, you will get no texgen and no lighting, which matches what is shown in the UI.

@sauraen
Copy link
Contributor

sauraen commented Jul 18, 2024

If you absolutely must have them visible--for example, because you want people to know where the texgen option is while they have lighting disabled, otherwise they can't find it--then I'd suggest you change the code so that instead of hiding the currently-invalid options, it grays them out (disables them) instead. Though that might be more confusing, as in our example texgen would be grayed out and checked, which has the semantic meaning of "this is required and you can't turn it off", rather than "this is stuck on but should not be".

@Lilaa3
Copy link
Collaborator Author

Lilaa3 commented Jul 18, 2024

I think disabling works quite well, the identation makes whats happening pretty obvious I think, decided to not disable in world defaults and fixed my typo in the node update (g_texture_gen instead of g_tex_gen)

@Lilaa3
Copy link
Collaborator Author

Lilaa3 commented Jul 18, 2024

Also world defaults ui is a broken in EX3 #370

Comment on lines -419 to -420
if not getattr(settings, textOrProp):
return None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enable should not be a parameter, enable should be coming from this, that's the whole point of this function, to enable/disable the group based on whether the head of the group is enabled.

Copy link
Contributor

@sauraen sauraen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix one tiny issue, otherwise approved. I'm still not sure this is ideal but if it unblocks the world defaults stuff then it's okay.

c.prop(settings, "g_fresnel_color")
else:
inputGroup.column().label(text=f"Shade color = {shadeColorLabel}")
inputGroup.label(text=f"Shade color = {shadeColorLabel}:")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the colon from the end of this line

@jesusyoshi54 jesusyoshi54 added the merge soon Will be merged in a few days at most if nothing else comes up label Jul 21, 2024
@Lilaa3 Lilaa3 merged commit ce37524 into Fast-64:main Jul 24, 2024
1 check passed
@Lilaa3 Lilaa3 deleted the show-all-rdp-settings branch August 24, 2024 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge soon Will be merged in a few days at most if nothing else comes up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants